Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable HIDE_SYMBOLS_BY_DEFAULT + linux patches #2248

Merged
merged 4 commits into from
Dec 18, 2023
Merged

Enable HIDE_SYMBOLS_BY_DEFAULT + linux patches #2248

merged 4 commits into from
Dec 18, 2023

Conversation

j-rivero
Copy link
Contributor

🎉 New feature

Summary

Part of gazebosim/gz-cmake#392 and gazebosim/gz-cmake#166.

Test it

The PR enables the HIDE_SYMBOLS_BY_DEFAULT option and patch the failures found during the build on Linux.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@j-rivero
Added missing visible attributes
d7605a4 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Enable HIDE_SYMBOLS_BY_DEFAULT
f4ec310 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero j-rivero requested a review from mjcarroll as a code owner November 16, 2023 18:15
@j-rivero j-rivero changed the title Jrivero/missing visible declarations Enable HIDE_SYMBOLS_BY_DEFAULT + linux patches Nov 16, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 16, 2023 •
edited
Loading

Codecov Report

Attention: 203 lines in your changes are missing coverage. Please review.

Comparison is base (5d1c505) 64.75% compared to head (66cc09f) 65.66%.
Report is 124 commits behind head on main.

Files Patch % Lines
python/src/gz/sim/Light.cc 45.34% 47 Missing ⚠️
python/src/gz/sim/Link.cc 50.00% 46 Missing ⚠️
python/src/gz/sim/Joint.cc 50.00% 38 Missing ⚠️
python/src/gz/sim/World.cc 48.93% 24 Missing ⚠️
python/src/gz/sim/Model.cc 51.11% 22 Missing ⚠️
python/src/gz/sim/Actor.cc 52.77% 17 Missing ⚠️
python/src/gz/sim/Sensor.cc 60.86% 9 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2248      +/-   ##
==========================================
+ Coverage   64.75%   65.66%   +0.90%     
==========================================
  Files         357      324      -33     
  Lines       29144    30937    +1793     
==========================================
+ Hits        18872    20314    +1442     
- Misses      10272    10623     +351

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mjcarroll mjcarroll self-assigned this Nov 20, 2023
@mjcarroll
Copy link
Contributor

Looks like there are some bad interactions with Qt on Windows:

C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\moc_JointPositionController.cpp(78,1): warning C4273: 'gz::sim::gui::JointsModel::qt_static_metacall': inconsistent dll linkage [C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\JointPositionController.vcxproj]

C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\../../../../../../gz-sim/src/gui/plugins/joint_position_controller/JointPositionController.hh(42,5): message : see previous definition of 'qt_static_metacall' (compiling source file C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\moc_JointPositionController.cpp) [C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\JointPositionController.vcxproj]

C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\moc_JointPositionController.cpp(92,84): warning C4273: 'staticMetaObject': inconsistent dll linkage [C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\JointPositionController.vcxproj]

C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\../../../../../../gz-sim/src/gui/plugins/joint_position_controller/JointPositionController.hh(42,5): message : see previous definition of 'public: static QMetaObject const gz::sim::gui::JointsModel::staticMetaObject' (compiling source file C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\moc_JointPositionController.cpp) [C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\JointPositionController.vcxproj]

C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\moc_JointPositionController.cpp(92,84): error C2491: 'gz::sim::gui::JointsModel::staticMetaObject': definition of dllimport static data member not allowed [C:\J\workspace\gz_sim-pr-win\ws\build\gz-sim9\src\gui\plugins\joint_position_controller\JointPositionController.vcxproj]

Linux appears to be fine, though (just a known-flaky test)

@j-rivero
Set custom plugin visibility definitions
5bd0f73 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Make the linter happy
66cc09f 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Copy link
Contributor Author

Builds are fine in the three platforms but there are some warnings on Windows and test failures on Linux and Mac that does not sound to me like related to these changes.

@azeey
Copy link
Contributor

azeey commented Dec 15, 2023

Builds are fine in the three platforms but there are some warnings on Windows and test failures on Linux and Mac that does not sound to me like related to these changes.

Yup, I think the test failures on Linux and Mac are known issues. I believe the warnings on windows were fixed on gz-sim8.

azeey
azeey approved these changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! But do you know why some plugins need to export symbols while others don't?

@j-rivero
Copy link
Contributor Author

LGTM! But do you know why some plugins need to export symbols while others don't?

That is a good question now that I'm looking at it slowly without the patching hammer on. Windows problems seems to appear on plugins when I added the GZ_SIM_GUI_VISIBLE keyword to them. The problems appeared on the moc_ generation so I assumed that it has something to do with that qt moc generation but I did not find any particular reason why this happens on the plot3D plugin for example.

I've removed the visibility key from both in a custom branch: Build Status let's see what happen.

@j-rivero
Copy link
Contributor Author

I've removed the visibility key from both in a custom branch: Build Status let's see what happen.

Windows did not have problems on building without plugin keywords. However I tried the same on Linux and that failed with missing symbols while compiling the gui TEST files:

 src/gui/plugins ❯ find . -name *_TEST*                                                                                                                                                                                                                                                                           
  ./plot_3d/Plot3D_TEST.cc                                                                                                                                                                                                                                                                                                                                                                    
  ./joint_position_controller/JointPositionController_TEST.cc

The PR patches the two plugins because we are only implementing _TEST on both so we only need binary symbols on these two. By now.

@j-rivero j-rivero merged commit 451dcf3 into main Dec 18, 2023
4 of 8 checks passed
@j-rivero j-rivero deleted the jrivero/missing_visible_declarations branch December 18, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azeey azeey azeey approved these changes

@mjcarroll mjcarroll Awaiting requested review from mjcarroll mjcarroll is a code owner

Assignees

@mjcarroll mjcarroll

Labels
None yet
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@j-rivero @mjcarroll @azeey